Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: apply clang-tidy rule performance-unnecessary-value-param #26042

Closed
wants to merge 1 commit into from
Closed

src: apply clang-tidy rule performance-unnecessary-value-param #26042

wants to merge 1 commit into from

Conversation

gengjiawen
Copy link
Member

Fix clang-tidy issue https://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-value-param.html.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 11, 2019
refack
refack reviewed Feb 11, 2019
View reviewed changes
Copy link
Contributor

@refack refack left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ambivalent of this change...
IMHO it has pros:

  1. Use of clang-tidy as an automated semantic tool
  2. Improve semantics of some APIs
  3. possible performance improvement

Cons:

  1. One-time use of automated tool (will regress without CI)
  2. Impair semantics of some APIs (esp. WRT to shared_ptr)
  3. performance impact non obvious. Adding <utility> has compile time impact.
  4. non K.I.S.S. use of value semantics

src/inspector/main_thread_interface.cc Outdated Show resolved Hide resolved
src/inspector/main_thread_interface.cc Outdated Show resolved Hide resolved
@refack
Copy link
Contributor

refack commented Feb 11, 2019 •
edited
Loading

@gengjiawen I like the idea of using clang-tidy. I'm not 100% sure about this specific implementation...

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2569/

@gengjiawen
Copy link
Member Author

@refack Maybe start an issue to start talk about clang-tidy. I am using Clion (bundled with clang-tidy) to review nodejs cpp code :)

@refack
Copy link
Contributor

refack commented Feb 11, 2019

Maybe start an issue to start talk about clang-tidy. I am using Clion (bundled with clang-tidy) to review nodejs cpp code :)

I use MSVS2017/9 which also has builtin clang-tidy integration (e.g. #23793). I also use Resharper (also by JetBrains) and sometimes I use CLion. I agree that proper use of better tools should allow us to improve our codebase.

We have had a bit of discussion around clang-tidy - https://github.com/nodejs/node/search?q=clang-tidy&type=Issues
I think in general the mood is positive, but IIUC there is no automatic consensus around it ¯\(ツ)/¯

I'd be happy to hear other contributors' opinions.

@gengjiawen
Copy link
Member Author

I found MSVS2017 is pretty lame in handling cpp tbh.

@gengjiawen
Copy link
Member Author

@addaleax Any thought on this clang-tidy rule ?

@addaleax
Copy link
Member

@gengjiawen I don’t know … if the compiler can optimize this away (and it should), then yeah, I don’t know and I don’t think I have much to add to what the others here have said.

I do like the changes to node_url.h and the cctest, and I’d definitely want to keep them.

@gengjiawen
Copy link
Member Author

Should we start an issue to discuss https://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-value-param.html with node cpp team (I am not sure it exists) ?

Or just keep the change node_url.h and cctest, and revert others ?

Thoughts ? cc @refack @addaleax

@addaleax
Copy link
Member

Should we start an issue to discuss https://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-value-param.html with node cpp team (I am not sure it exists) ?

I think this PR is that issue :)

I have no strong feelings either way.

@refack
Copy link
Contributor

refack commented Feb 14, 2019 •
edited
Loading

I'm +1 on the const std::string& changes (since std::string is mutable the const is a nice bonus).

I'm -0 on the changes that add std::moveing of smart pointers in constructors.
IIUC the elision of their redundant re-construction is creeping into the ISO standard as mandatory anyway.

I'm -1 on changes that put const std::shared_ptr<T>& arg_ in the signature of APIs that actually do take a copy of the pointer.

@gengjiawen
Copy link
Member Author

@refack I will try to do revert the const std::shared_ptr<T>& arg_ part tomorrow.

@gengjiawen
Copy link
Member Author

@refack @addaleax Reverted the const std::shared_ptr<T>& arg_ part.

@addaleax
Copy link
Member

addaleax commented Feb 15, 2019 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/20799/ (:heavy_check_mark:)

@gengjiawen
Copy link
Member Author

@danbev Can you import this change ? Thanks.

@danbev
Copy link
Contributor

danbev commented Feb 19, 2019

Landed in 8375c70.

@danbev danbev closed this Feb 19, 2019
danbev pushed a commit that referenced this pull request Feb 19, 2019
@gengjiawen @danbev
src: apply clang-tidy rule performance-unnecessary-value-param
8375c70 
PR-URL: #26042
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gengjiawen
Copy link
Member Author

Thanks.

@gengjiawen gengjiawen deleted the clang-tidy-perf-value-param branch February 19, 2019 12:17
addaleax pushed a commit that referenced this pull request Feb 19, 2019
@gengjiawen @addaleax
src: apply clang-tidy rule performance-unnecessary-value-param
79376ce 
PR-URL: #26042
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack refack mentioned this pull request Feb 25, 2019
Merged
2 tasks
@BridgeAR BridgeAR mentioned this pull request Feb 26, 2019
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@gengjiawen @rvagg
src: apply clang-tidy rule performance-unnecessary-value-param
25ddbc9 
PR-URL: #26042
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack left review comments

@bnoordhuis bnoordhuis bnoordhuis left review comments

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gengjiawen @refack @addaleax @danbev @bnoordhuis @jasnell @nodejs-github-bot